Property support: Use static unmodifiable Maps for common defaults#2041
Conversation
0828d57 to
4d8d0f1
Compare
Goal: Decrease memory usage when lots of property supports are in use. 449281
4d8d0f1 to
7a101bf
Compare
paolobazzi
left a comment
There was a problem hiding this comment.
See inputs for minor improvements
| public class DefaultValueMapTest { | ||
|
|
||
| @Test | ||
| public void testIsEmptyAndSize() { |
There was a problem hiding this comment.
Add test methods for non-covered parts:
- Constructor using startEmpty=true
- containsKey
- containsValue
- size
- isEmpty
| }.init(); | ||
| P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst(); | ||
| Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues(); | ||
| assertEquals(Set.of( |
There was a problem hiding this comment.
maybe add a message like 'found additional initally set property, check factory method, see org.eclipse.scout.rt.client.ui.BasicPropertySupportFactory'
| /** | ||
| * Factory to create a {@link BasicPropertySupport} for any {@link AbstractPropertyObserver}. | ||
| * <p> | ||
| * The newly created property support will always be backed by an underlying empty {@link Map}, this is either a regular {@link HashMap} or a {@link DefaultValueMap} which is size-optimized for the specific property observer if it is filled |
There was a problem hiding this comment.
underlaying map is not per-se empty if it was initialized with default values?
| } | ||
|
|
||
| protected Map<String, Object> createDefaultValueMapForAbstractButton() { | ||
| HashMap<String, Object> defaultValues = new HashMap<>(createDefaultValueMapForAbstractFormField()); |
There was a problem hiding this comment.
why does the methods mix up two init pattern (new ImmutablePair() vs. defaultValues.put) ?
-> use defaultValues.put() for all?
| public BasicPropertySupport createFor(AbstractPropertyObserver holder) { | ||
| // order matters | ||
| Map<String, Object> defaultValues = switch (holder) { | ||
| case AbstractMenu ignored -> m_defaultValuesByClass.computeIfAbsent(AbstractMenu.class, k -> createDefaultValueMapForAbstractMenu()); |
| boolean additionalValuesContainsKey = m_additionalValues.containsKey(key); | ||
| if (m_defaultValues.containsKey(key)) { | ||
| if (isDefaultValue(key, value)) { // must be same, equals would not be sufficient | ||
| return additionalValuesContainsKey |
There was a problem hiding this comment.
code is hard to read -> split into several methods or use if then else on separate lines and prepending a javadoc indicating which case is handled
e.g.:
@Override
public Object put(String key, Object value) {
boolean additionalValuesContainsKey = m_additionalValues.containsKey(key);
if (m_defaultValues.containsKey(key)) {
if (isDefaultValue(key, value)) { // must be same, equals would not be sufficient
if (additionalValuesContainsKey) {
Object removedValue = m_additionalValues.remove(key);
return removedValue != REMOVED_MARKER ? removedValue : null;
} else {
return m_defaultValues.get(key);
}
}
else if (!additionalValuesContainsKey) {
m_additionalValues.put(key, value);
return m_defaultValues.get(key); // load previous value from default values (if any)
}
}
//....
|
|
||
| @Override | ||
| public Set<Entry<String, Object>> entrySet() { | ||
| return new AbstractSet<>() { |
There was a problem hiding this comment.
extract into protected sub-class for maintenance/override?
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class DefaultValueMapTest { |
There was a problem hiding this comment.
check for missing coverage in DefaultValueMap:
- Iterator/EntrySet.next()
- Iterator/EntrySet.remove()
- Iterator/EntrySet.size
- Iterator/EntrySet.isEmpty
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class DefaultValueMapTest { |
There was a problem hiding this comment.
do we need a platformtestrunner if no BEANS are used?
| mapWithA.put("B", 2); | ||
| mapWithA.put("C", 3); | ||
|
|
||
| mapWithA.keySet().remove("A"); |
Goal: Decrease memory usage when lots of property supports are in use.
449281